-
Notifications
You must be signed in to change notification settings - Fork 197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proposing a get endpoint for Take Screenshot that with fullpage option #1536
base: master
Are you sure you want to change the base?
Conversation
Due to the high demand from https://bugs.chromium.org/p/chromedriver/issues/detail?id=294, come up with initial proposal for the spec
geckodriver supports this, but it's a new endpoint rather than a parameter ( |
Thanks for the comment, made the change accordingly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that merging this also requires some web-platform-tests to be submitted/approved for the feature.
index.html
Outdated
<li><p>Return {<var>x</var>, <var>y</var>, <var>width</var>, <var>height</var>}. | ||
</ol> | ||
|
||
<p>In order to <dfn>draw a bounding box from the fullpage region</dfn>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this just reuse the "draw a bounding box from the framebuffer" steps? It seems to be mostly duplicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The definition of "draw a bounding box from the framebuffer" uses initial viewport to define the width and height of the paint which is not the case here since we want the width and height of the entire page instead of just visible area.
index.html
Outdated
|
||
<li><p>Let <var>width</var> be the value | ||
returned by | ||
<p><a>max</a>(document.body.scrollWidth, document.documentElement.scrollWidth, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there really no defined relations that enforce an ordering between any of these properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what you mean regarding ordering of the properties. I took this "heuristics" from https://javascript.info/size-and-scroll-window. In reality, ChromeDriver implementation will probably utilize CDP https://chromedevtools.github.io/devtools-protocol/tot/Page/#method-getLayoutMetrics and use the contentSize width and height directly.
Will work on web-platform-tests once I have "verbal" approval for this. |
Hi @k7z45 — in order to accept this PR we need to make our tooling recognize that you’re already a participant in the Browser Testing and Tools Working Group. You can do that by using https://www.w3.org/users/myprofile to log into your W3C account — and from there, follow the Connected Accounts link in the sidebar to connect your W3C account with your GitHub account. If you don’t remember your W3C username or password, you can use https://www.w3.org/accounts/recover to recover them. |
Thanks Michael! Just did that. |
Thanks @k7z45 — https://labs.w3.org/repo-manager/pr/id/w3c/webdriver/1536 shows this is now green for the IPR check |
This commit is to add tests using GET screenshot/full for WebDriver proposed at [1]. The tests are copied from take_screenshot/ and modified to use the full page screenshot endpoint. In iframe.py test_source_origin test, the cross origin test full page screenshot does not generate the same png as the same origin one. This behavior has been verified by taking the full page screenshot in browser DevTools directly using Capture full size screenshot[2]. Added a function to get full page dimensions using document.scrollingElement.scroll{Width,Height}. [1] w3c/webdriver#1536 [2] https://www.deconetwork.com/blog/how-to-take-full-webpage-screenshots-instantly/#:~:text=Click%20Command%2BShift%2BP%20on,Select%20Capture%20full%20size%20screenshot.
Hi @jgraham , |
@@ -9082,6 +9090,45 @@ <h2>Screen capture</h2> | |||
<li><p>Return <a>success</a> with <var>canvas</var>. | |||
</ol> | |||
|
|||
<p>In order to <dfn>draw a scrollable region from the framebuffer</dfn>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly is that part about? Is it for the document element, or a child node? For child nodes we have Take Element Screenshot
. Otherwise check you do below we already have at the top of the former list.
I feel we don't have to repeat all the steps again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for describing taking the screenshot for the full page instead of only the visible viewport.
It's for document.scrollingElement.
I did not use "draw a bounding box from the framebuffer" because I think step 2 and step 3
do not satisfy the requirement for full page screenshot.
"2. Let paint width be the initial viewport’s width – min(rectangle x coordinate, rectangle x coordinate + rectangle width dimension).
3. Let paint height be the initial viewport’s height – min(rectangle y coordinate, rectangle y coordinate + rectangle height dimension)."
Notice the - denotes the minus sign, so I think for Take Screenshot and Take Element Screenshot, the width and height will not be greater than the visible viewport's width and height but the full page screenshot takes the document.scrollingElement.scrollWidth and scrollHeight which can be greater. So I removed step 2 and 3 and uses the scrollWidth and Height directly for paint width and height.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@whimboo Could you take a look, please?
I'm interested in bringing this functionality to WebDriver BiDi. Whether it belongs here in WebDriver "classic" is not a topic I can comment on, but I'm interested in learning if the normative text proposed here is viable for either spec. Specifically, I haven't found any implementer's perspective on @jgraham's 2021 comment:
@chrishtr you and I investigated a Chrome rendering bug a number of years ago; is there any chance you could weigh in here or perhaps refer me to someone more appropriate? @juliandescottes could you direct me to a subject-matter expert at Mozilla? |
For Chrome there is https://bugs.chromium.org/p/chromium/issues/detail?id=770769&desc=2 which at least for headless seems to still limit the maximum height to 16384px. For Firefox we currently have the limit set to 32767px for both height and width, and a maximum texture size of 472907776. As of now we do not have problems in capturing full page screenshots for pages exceeding the visible viewport. |
Note that we will discuss the full screenshot feature for BiDi during the TPAC sessions on Thursday or Friday this week. |
Due to the high demand from
https://bugs.chromium.org/p/chromedriver/issues/detail?id=294,
come up with initial proposal for the spec
Preview | Diff